codec, table: make new collation setting explicit in encoding (#69566)#69685
Conversation
ti-chi-bot
added
release-note-none
📝 WalkthroughWalkthroughIntroduces a ChangesCollation-aware encoding refactor
Estimated code review effort: 4 (Complex) | ~75 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Tools execution failed with the following error: Failed to run tools: 13 INTERNAL: Received RST_STREAM with code 2 (Internal server error) Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/table/tables/index.go (1)
63-70: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUpdate the now-stale
initNeedRestoreDatacomment.The comment states the lazy
sync.Onceis required becauseneedRestoredDatarelies onNewCollationEnabled()and the collation global is initialized afterNewIndex(). With this change, the collation mode is captured eagerly intoencoderat construction, andGenIndexValuecomputesneedRestoredDatafrom the already-fixedc.encoder.UseNewCollate(). Thesync.Oncetherefore no longer defers reading the global (its stated justification); it now only avoids recomputation. Please revise the comment so it doesn't mislead future readers about the collation ordering invariant.As per coding guidelines: "Comments SHOULD explain non-obvious intent, constraints, invariants... SHOULD NOT restate what the code already makes clear".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/table/tables/index.go` around lines 63 - 70, Update the stale comment on the initNeedRestoreData field in index state so it matches the current behavior: it should explain that sync.Once is only used to avoid recomputing needRestoredData, not because NewCollationEnabled() must be deferred until after NewIndex(). Refer to initNeedRestoreData, needRestoredData, encoder, and GenIndexValue in the comment text so it reflects that the collation choice is now captured eagerly in the constructor and read from c.encoder.UseNewCollate().Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/table/tables/index.go`:
- Around line 63-70: Update the stale comment on the initNeedRestoreData field
in index state so it matches the current behavior: it should explain that
sync.Once is only used to avoid recomputing needRestoredData, not because
NewCollationEnabled() must be deferred until after NewIndex(). Refer to
initNeedRestoreData, needRestoredData, encoder, and GenIndexValue in the comment
text so it reflects that the collation choice is now captured eagerly in the
constructor and read from c.encoder.UseNewCollate().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a953bec0-b21b-483b-937e-e3bdd4b56828
📒 Files selected for processing (41)
pkg/ddl/column.gopkg/ddl/index.gopkg/executor/admin.gopkg/executor/test/executor/BUILD.bazelpkg/executor/test/executor/executor_test.gopkg/executor/write.gopkg/expression/expression.gopkg/meta/model/table.gopkg/planner/core/expression_rewriter.gopkg/server/handler/tests/BUILD.bazelpkg/server/handler/tests/http_handler_test.gopkg/session/BUILD.bazelpkg/session/global_init.gopkg/session/session.gopkg/store/mockstore/BUILD.bazelpkg/store/mockstore/cluster_test.gopkg/store/mockstore/unistore/cophandler/cop_handler_test.gopkg/table/tables/index.gopkg/table/tables/mutation_checker.gopkg/table/tables/mutation_checker_test.gopkg/table/tables/partition.gopkg/table/tables/tables.gopkg/table/tables/testutil/BUILD.bazelpkg/table/tables/testutil/indexcheck.gopkg/table/tblctx/BUILD.bazelpkg/table/tblctx/buffers.gopkg/table/tblctx/buffers_test.gopkg/tablecodec/tablecodec.gopkg/tablecodec/tablecodec_test.gopkg/testkit/mockstore.gopkg/types/etc.gopkg/util/codec/codec.gopkg/util/codec/codec_test.gopkg/util/codec/collation_test.gopkg/util/collate/collate.gopkg/util/rowDecoder/BUILD.bazelpkg/util/rowDecoder/decoder_test.gopkg/util/rowcodec/bench_test.gopkg/util/rowcodec/rowcodec_test.gotests/realtikvtest/addindextest2/BUILD.bazeltests/realtikvtest/addindextest2/global_sort_test.go
💤 Files with no reviewable changes (1)
- pkg/meta/model/table.go
|
/retest |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## release-nextgen-202603 #69685 +/- ##
===========================================================
Coverage ? 76.1626%
===========================================================
Files ? 1936
Lines ? 540560
Branches ? 0
===========================================================
Hits ? 411705
Misses ? 128855
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
ti-chi-bot
Bot
added
lgtm
and removed
needs-1-more-lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: D3Hunter, GMHDBJD, joechenrh, qw4990, windtalker The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
ti-chi-bot
Bot
merged commit 8b1d326
into
pingcap:release-nextgen-202603
6 participants
This is an automated cherry-pick of #69566
What problem does this PR solve?
Issue Number: ref #69563
Problem Summary:
Some lower-level codec and tablecodec helper paths read the global new-collation setting directly when deciding how to encode values or whether restored data is needed. That makes deep utility behavior depend on process-wide state and makes the new-collation global harder to eliminate incrementally.
This PR is the first part of the refactor. It removes the implicit global dependency from the encoding paths touched here by threading an explicit collation setting through the relevant table, tablecodec, and codec helpers. Follow-up work should continue eliminating the remaining global variable usage.
in the first phase, we we will only refactor this part to make sure nextgen addindex/import-into can work in the case in the issue
What changed and how does it work?
codec.Encoder, which stores the new-collation setting used byEncodeKey,EncodeValue, andHashCode.codec.Encoderthrough row and index encoding helpers intablecodec,table/tables, andtable/tblctx.NeedRestoredDataWithCollatewhen the collation setting is already known.Check List
Tests
Unit tests:
./tools/check/failpoint-go-test.sh pkg/util/codec -run TestEncoderNewCollationEnabled -count=1./tools/check/failpoint-go-test.sh pkg/tablecodec -run 'Test(RowCodec|DecodeColumnValue|TimeCodec|CutRow|UniqueGlobalIndexKeyWithNullValues)$' -count=1./tools/check/failpoint-go-test.sh pkg/table/tables -run 'Test(CheckRowInsertionConsistency|CheckIndexKeysAndCheckHandleConsistency)$' -count=1pushd pkg/table/tblctx >/dev/null && go test -run 'Test(EncodeRow|EncodeBufferReserve)$' -tags=intest,deadlock -count=1 && popd >/dev/nullManual validation:
make bazel_preparegit diff --check master...HEADmake lintSide effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit
New Features
Bug Fixes